-
Notifications
You must be signed in to change notification settings - Fork 91
Rewrite for Xcode 9 & Swift 3.2 #84
base: development
Are you sure you want to change the base?
Conversation
@danthorpe getting lots of Also |
@attheodo - pull, & try again? |
@danthorpe forgot to mention: you probably want to declare Also, can you quickly take care of |
@attheodo - done, try again? |
@danthorpe yeap, things look good on my end! Thanks for your effort mate. 👍 |
Is there any reason to still use |
@lmcd no, and in fact These are internal implementation details which will change here when I do the formatting stuff. |
Is there an expected merge date for this PR? I'm using Money and it currently breaks build. :/
|
Also, what changed for Money? Because I used to be able to use strings as such: And the text would be converted to "$10.20" no problem. Now, however, it just prints:
Am I SOOL with Swift 3.2/4 and should rollback to 3? Edit: NVM. It seems locale is part of the stuff that Swift 4 broke... |
@aravasio this branch is a rewrite of Money. Looks like |
@danthorpe any updates on the |
Sources/CurrencyProtocol.swift
Outdated
|
||
func numberFormatter(withStyle style: NumberFormatter.Style, forLocaleId localeId: String) -> NumberFormatter { | ||
let canonicalId = Locale.canonicalIdentifier(from: localeId) | ||
let locale = Locale(identifier: "\(canonicalId)@currency=\(code))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one closing parentheses too many in the identifier here. It's causing some funky issues like getting the JPY symbol instead of the € symbol in some places etc.
This line should be:
let locale = Locale(identifier: "\(canonicalId)@currency=\(code)")
I apologize if this has been addressed, but my amounts are rounding to the nearest dollar. For example 15.78 would be 16. |
Seems like this is an undocumented change to how NumberFormatter works possibly.
@clintonmedbery try now? |
@danthorpe Nope, sorry. Was rounding up, now I get AFA15 instead of 15 when I should get 14.78. Sorry... |
@danthorpe nope, still doesn't look good. Getting |
Re-implements `.Local` class
So, what's important to know when using the If you have found that you get a currency reporting as Likewise, if you are finding that the For |
I put together a test that shows a couple of assertions that don't quite work the way I would expect. I don't print them out here, but in each case, the .currency property of the Money object is 'USD', with a scale of 2, as I would expect:
Am I correct in my assumptions here, and the use of the Money type? |
@mattcorey well - I understand where you're coming from. And to aid the discussion, I've re-written your test as though it was a unit test: extension MoneyTestCase {
func test__calculation1() {
let balance: Money = 5000
let payment: Money = 100
let apr = 0.20
let monthlyInterestRate = apr / 12.0
let interest = balance * monthlyInterestRate
let principal = payment - interest
let newBalance = balance - principal
print("interest: \(interest.decimal)")
print("principal: \(principal.decimal)")
print("newBalance: \(newBalance.decimal)")
XCTAssertEqual(8333, interest.minorUnits)
XCTAssertEqual(83.33 as Money, interest)
XCTAssertEqual(1667, principal.minorUnits)
XCTAssertEqual(16.67 as Money, principal)
XCTAssertEqual(498333, newBalance.minorUnits)
XCTAssertEqual(4983.33 as Money, newBalance)
}
} And this is the output on the console (slightly modified to remove file paths)
So, what's going on here?
So, what does this mean right now?
What does this mean going forward?
|
@danthorpe Excellent description - thanks! I expected that most of the assert equals were failing because of an underlying data store, but your explanation lends some nuance to my understanding. Ideally, I think the type should provide ‘intuitive’ behaviors for things likely rounding and equality - to me, “Bankers Rounding” and equality on the localized representation seem the most intuitive, but opinions would likely vary (I’m not sold on the equality behavior myself, but it’s what comes to mind first when I consider how I think it should work) As for the 271, I’m glad you got the same result, because I was questioning my basic understanding of math on that one :) |
Is this stable now? Was the issue with |
@lmcd the issue with the Will also address the issues that @mattcorey has helped raise in the tests before this is merged. |
Is this going to be merged? |
Any update on this? |
@mattcorey @danthorpe The 271 is from an issue with NSDecimalNumber breaking on values with a large amount of decimal places when you try to get the intValue. The proper solution is to get doubleValue and cast to Int |
@tplester Interesting - is this issue documented anywhere? @danthorpe I’m assuming this would need to be handled internally - are you still active with the library? |
@tplester - That ticket claims to be resolved in December of 2016 - is it still an issue? |
@mattcorey The ticket says its resolved but the bug is still there in Xcode 9. var num = NSDecimalNumber(value: 100)
num = num.dividing(by: NSDecimalNumber(value: 3))
num.int32Value
num.intValue
num.int64Value
Int(num.doubleValue) |
@danthorpe @tplester From yesterday itself, I'm trying to update or install Money framework into my project. I'm using "Pod install Money" command from terminal, then it's showing as [!] Unknown command:
None of your spec sources contain a spec satisfying the dependency: You have either:
Note: as of CocoaPods 1.0, Can you help me on this |
@SubbaNelakudhiti Please stick to your other open issue. The message says
Did you try this? Please answer in #91. |
Hi @danthorpe! I'm using this feature branch for one of my projects and would love to get it merged in. How can I help? |
Hey all - I would suggest migrating to https://github.com/Flight-School/Money - looks like a carbon copy API wise - and it's new so written for Swift 4 🚀 |
Xcode 9 snuck in some quite interesting breaking changes with Swift 3.2 and numeric protocols. In particular
Numeric
,SignedNumeric
,FloatingPoint
andSignedNumber
which no longer exists properly.Additionally,
Decimal
is now a value type bridged toNSDecimalNumber
but which does not provide a full implementation ofFloatingPoint
yet, but it does still expose decimal math operations (methods are now deprecated in favour of operators). TheDecimal
operators use.plain
rounding mode.So, this PR is pretty much the start of a complete re-write of Money, and what will eventually become v3. This PR is focused on getting core functionality working. To that end we have:
CurrencyProtocol
- protocol which provides currency description infoMoneyProtocol
- protocol which provides math functionalityMoney
which is for weakly typed currency (defaults to "device") - and provides an API entry point to eventually support dynamic money where the currency is unknown at compile time.ISOMoney
which is for money with a strongly typed currency, known at compile time. e.g.USD
, and should have the same API as v2 types.Autogenerated typealiases for ISO money
Localised formatting of Money values
Things which are currently missing